Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Generalize signature verifier of tendermint client. #958

Closed
wants to merge 1 commit into from
Closed

Generalize signature verifier of tendermint client. #958

wants to merge 1 commit into from

Conversation

riversyang
Copy link
Contributor

@riversyang riversyang commented Nov 9, 2023 •
edited
Loading

Description

While implementing near-ibc, we found that the gas consumption of signature verification in tendermint client was high (about 9T gas for each) because it was using a pure rust implementation of the verifier (tendermint::crypto::default::signature::Verifier). In NEAR protocol, we can use a precompiled function to implement the ed25519 signature verification, which will only cost about 0.3T gas for each. So I made a change to the current tendermint client implementation, which enables a customized verifier of the tendermint ClientState. The verifier will be specified by the host implementation of ibc-rs, rather than a default implementation inside tendermint ClientState.

Of course I also made necessary changes to related code and tests.

This implementation had been included in the latest near-ibc implementation which reduced the gas consumption of signature verifications for about 96%. Obviously a big optimization for near-ibc.

I think this is also worth to be checked in ibc-rs to see whether this change is good enough to be included in the following releases.

@Farhad-Shabani
Copy link
Member

Thanks @riversyang for opening this PR and the given context. 🌷
This week, we're working on organizing things in ibc-rs. Please give us some time to thoroughly investigate this issue, since it goes into intricate details.

@codecov Codecov
Copy link

codecov bot commented Nov 20, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (527bb14) 67.72% compared to head (4ad0b1b) 68.51%.
Report is 5 commits behind head on main.

Files Patch % Lines
...s/ibc/src/hosts/tendermint/validate_self_client.rs 0.00% 4 Missing ⚠️
...c/src/hosts/tendermint/upgrade_proposal/handler.rs 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #958      +/-   ##
==========================================
+ Coverage   67.72%   68.51%   +0.78%     
==========================================
  Files         130      130              
  Lines       16491    16320     -171     
==========================================
+ Hits        11168    11181      +13     
+ Misses       5323     5139     -184

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Farhad-Shabani Farhad-Shabani mentioned this pull request Nov 23, 2023
Closed
@Farhad-Shabani
Copy link
Member

Farhad-Shabani commented Nov 23, 2023 •
edited
Loading

Thanks again @riversyang for bringing this to our attention. It's an excellent example highlighting a potential issue with our current ClientState struct. The verifier seems out of place as a field there. The role of the verifier is instrumental in implementing the ClientState interface. As such, it would be more fitting to relocate its logic to the implementation layer (ibc-client-tendermint crate) rather than keeping it within the types (ibc-client-tendermint-types crate). This would be aligned with our new repository structure.

I've opened this issue. The solution, requires a bit more effort to properly detach the verifier from the ClientState struct, as detailed there. Otherwise we will be imposing unnecessary complexity to our implementations by introducing a generic V everywhere, which impedes the ease of integration. Particularly, most users rely on the default ProdVerifier.
We can close this PR if the given context makes sense to you.

@riversyang
Copy link
Contributor Author

Thank you @Farhad-Shabani for your prompt and detailed response. I completely agree with your perspective on aiming for a more generalized and simplified design for future use. Tracking this through the issue you created seems like the most efficient way forward. This pr can be closed.

@riversyang riversyang closed this Nov 23, 2023
@Farhad-Shabani Farhad-Shabani mentioned this pull request Nov 29, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
ibc-rs
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@riversyang @Farhad-Shabani